-
Notifications
You must be signed in to change notification settings - Fork 25.5k
ESQL: Timezone query setting and added to the query config #136205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.3.csv # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of comments
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/Configuration.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/Configuration.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QuerySettings.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QuerySettings.java
Outdated
Show resolved
Hide resolved
return new QuerySettingsMap(settings); | ||
} | ||
|
||
public static class QuerySettingsMap implements Writeable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The QuerySettings
class has become a class without instances. Why not re-use it for the map, rather than having a separate class that holds the map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll try to do that (in the next PR)
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QuerySettings.java
Show resolved
Hide resolved
|
||
private final Map<String, Expression> settings; | ||
|
||
public QuerySettingsMap(Map<String, Expression> settings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Any reason for this not being a Map<QuerySettingsDef, Expression>
? We could back this with an enum map to avoid looking things up by string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the Def has validation, parsing... It's similar to Settings, but for ESQL.
I created this class just to be transported to datanodes: Plain data, to be read by datanodes using their Defs.
As I'm removing transport for this PR, I'll rethink a bit those topics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine and the API you made is cool.
But, maybe looking up by string will be a little heavy; the querysettingsdefs are kinda just an enum and could be assigned ids as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could store it in the configuration later as the map you comment yes. Even without IDs, as they're static objects. It would matter at transport time though, but we're free to serialize as strings in there if we want.
So yep, I'll make this change in the next PR!
} | ||
} | ||
|
||
public <T> T get(QuerySettingDef<T> def, RemoteClusterService clusterService) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to pass in the RCS every time sounds wrong. Isn't this something that we'd only do once, when the query is first parsed and we read the QuerySetting
values from the parsed query?
That is, I'd expect that we use the RCS once while building the QuerySettingsMap, and after, the settings are fixed and passed around.
Otherwise, we'd have to pass the RCS to all functions that might want to get a setting value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the same. Theoretically, it will depend on how we store this in the config: If we store the original Expression, or the final value. This is still a bit abstract, as no actual config uses that yet though, so I'm not sure if this is required here to begin with. I could remove it from get() and keep it only in validate().
@luigidellaquila, some idea of what we need that for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or the final value
I think we should only store the final (Literal
or<T>
) value unless there's a good reason for settings to be complex enough that they have a dynamic value that can change where the setting is used. But even then, I think it still should be a final value and whatever is dynamic about it should be retrieved on the node where it's relevant.
I think the flow should be
- raw parsing from the query string
- validate + parse the actual settings, use RCS or pass in whatever else the settings need to be validated or parsed
- store the parsed result in a map that is passed on as part of the config.
"esql_configuration_query_settings" | ||
); | ||
|
||
private final Map<String, Expression> settings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd be a little more relaxed if this was restricted to Literal
s rather than arbitrary expressions ^^" Having an expression as setting sounds more complex than we currently need.
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
||
package org.elasticsearch.xpack.esql.plugin; | ||
|
||
public record EsqlQueryClusterSettings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a bundle of the required cluster settings used by the EsqlSession to create the configuration. We could also pass the cluster settings themselves and let the EsqlSession do it itself, but I wasn't sure if we wanted that. I'll evaluate the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The major changes here are:
- We now don't receive the config nor the optimizers in the constructor
- We instead create them in execute()
- So, we have to propagate them through methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Configuration" object wasn't being used here, at all. So I removed it. And this led to many changes along tests.
That's good btw, as otherwise creating the config after parsing would have been quite harder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I think the refactoring is super nice, thank you!
sessionId, | ||
configuration, | ||
foldCtx, | ||
new EsqlQueryClusterSettings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is more appropriate as name:
new EsqlQueryClusterSettings( | |
new AnalyzerSettings( |
Or QueryTruncationSettings
or so.
We also have PlannerSettings
that are obtained from the ClusterService
.
|
||
private final Map<String, Expression> settings; | ||
|
||
public QuerySettingsMap(Map<String, Expression> settings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine and the API you made is cool.
But, maybe looking up by string will be a little heavy; the querysettingsdefs are kinda just an enum and could be assigned ids as well.
} | ||
} | ||
|
||
public <T> T get(QuerySettingDef<T> def, RemoteClusterService clusterService) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or the final value
I think we should only store the final (Literal
or<T>
) value unless there's a good reason for settings to be complex enough that they have a dynamic value that can change where the setting is used. But even then, I think it still should be a final value and whatever is dynamic about it should be retrieved on the node where it's relevant.
I think the flow should be
- raw parsing from the query string
- validate + parse the actual settings, use RCS or pass in whatever else the settings need to be validated or parsed
- store the parsed result in a map that is passed on as part of the config.
This PR covers until right before the query setting is accessible from the Configuration.
PR roadmap
configuration.getSetting(QuerySettings.TIME_ZONE)